Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[luxtronikheatpump] Fix control signal circulating pump #15855

Conversation

jamietownsend
Copy link
Contributor

Fixes #15836

Copy link
Contributor

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jamietownsend
Thanks for providing this pull request. The Energy definition was for sure added by accident.

I'm actually not using this channel on my heatpump as it doesn't provide a value in my set up. Otherwise I might have noticed it.

I've added some comments, as some changes might be required to have that working correctly.

@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Nov 6, 2023
@jamietownsend jamietownsend force-pushed the 15836-Failed-to-update-item-ControlSignalCirculatingPump-after-upgrading-to-OH4 branch from 9990e89 to dc3ca47 Compare November 6, 2023 20:00
@lsiepel lsiepel changed the title [luxtronikheatpump] Fix issue with item control signal circulating pump after upgrading to OH4 [luxtronikheatpump] Fix control signal circulating pump Nov 6, 2023
@jamietownsend jamietownsend force-pushed the 15836-Failed-to-update-item-ControlSignalCirculatingPump-after-upgrading-to-OH4 branch from dc3ca47 to c352739 Compare November 6, 2023 21:54
@jamietownsend jamietownsend force-pushed the 15836-Failed-to-update-item-ControlSignalCirculatingPump-after-upgrading-to-OH4 branch from b5078f9 to bcafa9c Compare November 8, 2023 08:47
@jamietownsend jamietownsend force-pushed the 15836-Failed-to-update-item-ControlSignalCirculatingPump-after-upgrading-to-OH4 branch from bcafa9c to 7e61c94 Compare November 8, 2023 10:27
@jamietownsend
Copy link
Contributor Author

Thanks for the fix! Indeed this is the correct dimension which is used for updating the channel:

/**
* Control signal circulating pump
* (original: Steuersignal Umwälzpumpe)
*/
CHANNEL_HZIO_PWM(183, "controlSignalCirculatingPump", NumberItem.class, Units.PERCENT, false, null),

Since the channel type changed, you need to provide update instructions for the channel. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

A similar example can be found here:

<update-channel id="waterConsumption">
<type>miele:water-consumption</type>
</update-channel>

There is a slight doubt for me though. I see statically declared channels:

<channel id="controlSignalCirculatingPump" typeId="controlSignalCirculatingPump"/>

But also code that seems to dynamically add channels:

// create list with available channels
for (HeatpumpChannel channel : HeatpumpChannel.values()) {
Integer channelId = channel.getChannelId();
int length = channel.isWritable() ? heatpumpParams.length : heatpumpValues.length;
ChannelUID channelUID = new ChannelUID(thing.getUID(), channel.getCommand());
ChannelTypeUID channelTypeUID;
if (channel.getCommand().matches("^channel[0-9]+$")) {
channelTypeUID = new ChannelTypeUID(LuxtronikHeatpumpBindingConstants.BINDING_ID, "unknown");
} else {
channelTypeUID = new ChannelTypeUID(LuxtronikHeatpumpBindingConstants.BINDING_ID, channel.getCommand());
}
if ((channelId != null && length <= channelId)
|| (config.showAllChannels == Boolean.FALSE && !channel.isVisible(visibilityValues))) {
logger.debug("Hiding channel {}", channel.getCommand());
} else {
channelList.add(callback.createChannelBuilder(channelUID, channelTypeUID).build());
}
}
thingBuilder.withChannels(channelList);

The update instructions are not needed if the channel is actually added dynamically.

You can probably tell me where I am wrong. 🙂 But otherwise you could test this by first running the bundled version of the binding. Then create a managed Thing using the UI. Then uninstall the binding and drop your JAR with the fix. Then verify if the channel type is automatically updated after reinitialization, so that a new item will automatically be suggested as Number:Dimensionless. If it does, it looks like the channel is created dynamically. If not, most likely update instructions are needed. In this case you could add them and run the test again.

I'm not sure what needs to be done here. After following your suggested approach, I see that the Channel definition under the Thing shows "(Number:Dimensionless)", but the Item shows "Number:Energy". Deleting the Item and recreating it doesn't fix the problem. I notice the following while adding the Item from the Thing:
Screenshot 2023-11-08 at 12 20 08

Notice that the types are not the same. I also notice that the file .../openhab/userdata/jsondb/org.openhab.core.thing.Thing.json doesn't contain the correct value, even though the channel appears right on the Thing in the UI.

Deleting the Item and the Thing and then re-creating them solves the conflict.

@jlaur, any ideas how to proceed? (@sgiehl, FYI)

@jamietownsend
Copy link
Contributor Author

jamietownsend commented Nov 8, 2023

I added an update.xml to delete and re-create the controlSignalCirculatingPump channel. This enables the correct creation of a new Item. Old items will still have issues and will have to be manually deleted and re-created.

@jlaur
Copy link
Contributor

jlaur commented Nov 8, 2023

I added an update.xml to delete and re-create the controlSignalCirculatingPump channel. This enables the correct creation of a new Item. Old items will still have issues and will have to be manually deleted and re-created.

Yes, the update instructions will only update the channel definitions in jsondb, but will touch linked items.

Did you notice a difference between having the update instructions when upgrading the binding and not having them? Since I'm confused about whether this binding provide static or dynamic channels (or both), I would like to figure out if the instructions are needed in this case.

@jlaur
Copy link
Contributor

jlaur commented Nov 8, 2023

Did you notice a difference between having the update instructions when upgrading the binding and not having them? Since I'm confused about whether this binding provide static or dynamic channels (or both), I would like to figure out if the instructions are needed in this case.

If the channel was automatically updated for your existing Thing when you updated the binding, the instructions are not needed. You have not added the thingTypeVersion property, so the instructions haven't been triggered.

@jamietownsend
Copy link
Contributor Author

If the channel was automatically updated for your existing Thing when you updated the binding, the instructions are not needed.

This is the case. The Thing was updated automatically, even without the update.xml.

You have not added the thingTypeVersion property, so the instructions haven't been triggered.

Thanks for this clarification. I tested what happened when the thingTypeVersion property was correctly configured and I saw in the log that the update was correctly triggered and the thingTypeVersion changed from 0 to 1, but otherwise, there was no difference in what was changed. The Thing was updated, but the Item was not.

As such I'll remove the update.xml again to keep things simple.

Question: is there any way that we can automatically have the Items updated or should we write something in the docs to explain what has to be done?

@jamietownsend jamietownsend force-pushed the 15836-Failed-to-update-item-ControlSignalCirculatingPump-after-upgrading-to-OH4 branch from 58861af to d2694d4 Compare November 9, 2023 08:27
@jamietownsend
Copy link
Contributor Author

Not sure why this was closed. I'll try to re-open it...

Signed-off-by: Jamie Townsend <[email protected]>
@jamietownsend
Copy link
Contributor Author

Ok, re-opening 🙂

@jamietownsend jamietownsend reopened this Nov 9, 2023
@jlaur
Copy link
Contributor

jlaur commented Nov 9, 2023

Question: is there any way that we can automatically have the Items updated or should we write something in the docs to explain what has to be done?

There is no way to automatically update the items, but you could add a note somewhere around here:

https://github.com/openhab/openhab-distro/blob/b431198b21b0f435f62f5ebe671007dfafa7b53a/distributions/openhab/src/main/resources/bin/update.lst#L120-L122

These messages will be displayed during an upgrade.

@jamietownsend
Copy link
Contributor Author

Thanks @jlaur. See related pull request: openhab/openhab-distro#1606

I hope both pull requests can now me merged 🙂

Copy link
Contributor

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this looks good now.

Note: I'm unable to say if the new label matches what the channel actually provides. The previously used label was defined by "reverse engineering" and looking up what the java applet on the heatpump was using for it.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlaur jlaur merged commit fc67ec8 into openhab:main Nov 9, 2023
4 checks passed
@jlaur jlaur added this to the 4.1 milestone Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[luxtronikheatpump] Failed to update item ControlSignalCirculatingPump after upgrading to OH4
3 participants